Skip to content

Conversation

@frank-king
Copy link
Contributor

Part of pin_ergonomics. It forbids to impl Unpin for T where T is an ADT marked with #[pin_v2].

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 24, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 24, 2025

r? @madsmtm

rustbot has assigned @madsmtm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@madsmtm
Copy link
Contributor

madsmtm commented Dec 4, 2025

The change itself seems simple, but I'd prefer to have someone from t-types review it.
r? types

If no-one there feels like they have the capacity, feel free to assign it back to me.

@rustbot rustbot added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Dec 4, 2025
@rustbot rustbot assigned jackh726 and unassigned madsmtm Dec 4, 2025
@bors
Copy link
Collaborator

bors commented Dec 14, 2025

☔ The latest upstream changes (presumably #146348) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2025

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 7, 2026

r? BoxyUwU

@rustbot rustbot assigned BoxyUwU and unassigned jackh726 Jan 7, 2026
return Err(err.emit());
}

// Disallow explicit impls of the `Unpin` trait for structurally pinned types
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should live in a slightly difference place. This isn't enforcing anything related to the deny_explicit_impl stuff. It's also not really a general property of impls that we want to uphold- it's Unpin specific.

I think this should probably be a fn visit_implementation_of_unpin in coherehence/builtin.rs 🤔

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 7, 2026

What's the reason for wanting to forbid such impls? Is it just because it's probably pointless to have pin_v2 on an Unpin type, or is there some soundness concern here?

@frank-king
Copy link
Contributor Author

frank-king commented Jan 7, 2026

It is for soundness concern.

For a type T annotated with #[pin_v2], it is allowed to project Pin<&mut T> to its field Pin<&mut U> safely (even if U: !Unpin). If T is allowed to impl Unpin manually, then &mut U could be obtained from &mut T that dereferenced by Pin<&mut T>, which breaks the safety contract of Pin<&mut U> for U: !Unpin.

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 7, 2026

If its soundness critical, I would add tests for impls of unpin for type aliases for adts marked pin_v2, e.g. impl Unpin for <Foo as Identity>::Assoc impl Unpin for Identity<Foo> (with lazy_type_alias feature enabled) and impl Unpin for TAIT (where TAIT is Foo).

rn your logic implicitly ignores all the cases where the self type is a non-adt 🤔 which I think is fine because we enforce that impls of Unpin are always for ADTs? But having tests is good :) It would probably also be good to explicitly match on the TyKind and in non Adt cases (which we expect to be unreachable) delay a bug maybe 🤔 that way you don't get unsoundness here if it goes wrong just an ICE

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 19, 2026

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 19, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 19, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants